Cleanup make MacroElements from Elements #2066
Conversation
This change involves several classes that require being added to a Figure. Since these classes follow the `render` semantics of a MacroElement it makes more sense to make them also inherit from MacroElement. Excluded from this change is actually making use of the template mechanics of the MacroElement.
Conengmo
left a comment
Conengmo
left a comment
There was a problem hiding this comment.
Nice change! It's interesting how these classes are not exactly MacroElements either, since they don't use macros. But at least we get the return type of the render method correct.
I added two comments about the todo comments, after addressing those this should be good to merge.
Co-authored-by: Frank Anema <[email protected]>
Co-authored-by: Frank Anema <[email protected]>
|
Before you merge this, do you have some example usage of the |
|
We have examples for both classes in the Popup documentation: https://python-visualization.github.io/folium/latest/user_guide/ui_elements/popups.html#Vega-chart-in-popup |
Conengmo
left a comment
Conengmo
left a comment
There was a problem hiding this comment.
Code looks good! If you manually check these classes still work as expected, go ahead and merge it 👍 I'm less available next few days.
2 participants
This change involves several classes that require being added to a Figure. Since these classes follow the
rendersemantics of a MacroElement it makes more sense to make them also inherit from MacroElement.Excluded from this change is actually making use of the template mechanics of the MacroElement.